Keep the number of translations even with null values #427
Conversation
|
Thank you! |
|
@freekmurze This is a behavioral change that broke production for us. People using Edit: if someone else encounters the same issue, just lock the version down to 6.5.3. |
| @@ -65,7 +65,7 @@ public function getTranslation(string $key, string $locale, bool $useFallbackLoc | |||
|
|
|||
| $translations = $this->getTranslations($key); | |||
|
|
|||
| $translation = $translations[$normalizedLocale] ?? ''; | |||
| $translation = $translations[$normalizedLocale] ?? null; | |||
There was a problem hiding this comment.
This is a big issue for str functions that no longer accept null values and consequently blow up.
There was a problem hiding this comment.
Feel free to change it back to an empty string. Do make sure all tests pass.
There was a problem hiding this comment.
Every nullable varchar string from the database have to be checked before using the str functions. Also, with the previous version, null values were saved in the json field for the last language, so this issue was already there, am I wrong ?
There was a problem hiding this comment.
The previous behavior was converting those default null values to empty strings so this check was handled by the package, which was then changed to be null moving this burden onto the shoulders of the package users. 👍
|
That was also a critical change for us. Some of our tests in the CI failed. We have to change expect($estate)
->getTranslation('headline', 'de', false)->toBe('')to expect($estate)
->getTranslation('headline', 'de', false)->toBeNull() |
I've opened a PR that reverts this one. 👍 |
Currently, when saving this in the model:
…we have this in database:
This pull request fix this strange behavior and the data saved in the database is now:
Note : In the version 3.1.3 of this package, the number of translations where correctly maintained in the database, I don't know exactly when this behavior changed, but it can lead to bugs in some situations.